replace logind polling loops with signal subscriptions - #242
Open
omuhr wants to merge 1 commit into
Open
Conversation
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesSystem-event monitoring
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LidTask
participant LogindManager
participant PowerTask
participant UdevMioThread
participant Callbacks
LidTask->>LogindManager: read initial lid state
LogindManager-->>LidTask: send lid property changes
LidTask->>Callbacks: invoke callback for changed lid state
PowerTask->>PowerTask: discover mains supply and read online state
UdevMioThread->>PowerTask: deliver matching udev changes
PowerTask->>Callbacks: invoke callback for changed power state
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@asusd/src/lib.rs`:
- Around line 272-288: Skip the first cached event from both streams created by
receive_on_external_power_changed and receive_lid_closed_changed, then process
only subsequent change notifications in their respective tokio::spawn loops.
Preserve reload() as the startup-state handler and keep the existing callback
behavior for actual logind changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 23e44506-a1fc-43a8-94e4-00a2349ae9f8
📒 Files selected for processing (1)
asusd/src/lib.rs
📜 Review details
🔇 Additional comments (1)
asusd/src/lib.rs (1)
21-25: LGTM!Also applies to: 239-239
scardracs
suggested changes
Jul 30, 2026
|
|
||
| tokio::spawn(async move { | ||
| let mut last_lid = manager.lid_closed().await.unwrap_or_default(); | ||
| // need to loop on these as they don't emit signals |
Contributor
There was a problem hiding this comment.
On several Linux distributions, kernel configurations, and older systemd releases, systemd-logind does not consistently broadcast PropertiesChanged signals for OnExternalPower or LidClosed on the system D-Bus. OnExternalPower is populated via udev/sysfs power_supply rules, but logind's D-Bus interface vtable does not guarantee emits-change-signal="true" for all properties across all systemd versions. Relying solely on stream.next().await without fallback verification or udev event integration will cause asusd to stall indefinitely on systems where logind property signals do not fire.
Comment on lines
+241
to
+239
| .cache_properties(CacheProperties::No) | ||
| .cache_properties(CacheProperties::Yes) |
Contributor
There was a problem hiding this comment.
Setting CacheProperties::Yes instructs zbus to cache property values locally in memory. If a D-Bus signal is dropped, delayed, or un-emitted by systemd-logind, any call to change.get().await or manager.on_external_power().await will return stale cached values instead of querying systemd-logind directly over D-Bus. For a root-privileged system daemon managing thermal safety and power limits, relying on cached property states without forced invalidation risks operating on stale hardware assumptions.
Author
There was a problem hiding this comment.
Without CacheProperties enabled I don't get any trigger on lid state change.
It seems to me that the effect of the Cache is that it stores the value (if present) attached to the signal in memory, at least based on this comment on the .get() implementation for PropertyChanged:
impl<T> PropertyChanged<'_, T>
where
T: TryFrom<zvariant::OwnedValue>,
T::Error: Into<crate::Error>,
{
/// Get the value of the property that changed.
///
/// If the notification signal contained the new value, it has been cached already and this call
/// will return that value. Otherwise (i.e. invalidated property), a D-Bus call is made to fetch
/// and cache the new value.
pub async fn get(&self) -> Result<T> {
self.get_raw()
.await
.and_then(|v| T::try_from(OwnedValue::try_from(&*v)?).map_err(Into::into))
}
}So wouldn't the cached value correspond to the one communicated by the signal and not a stale value?
Comment on lines
236
to
242
| .expect("Controller could not create dbus connection"); | ||
|
|
||
| let manager = ManagerProxy::builder(&connection) | ||
| .cache_properties(CacheProperties::No) | ||
| .cache_properties(CacheProperties::Yes) | ||
| .build() | ||
| .await | ||
| .expect("Controller could not create ManagerProxy"); |
Contributor
There was a problem hiding this comment.
System daemons running as systemd root services must avoid .expect() and .unwrap() panics during D-Bus setup. If D-Bus is initializing asynchronously during early system boot, asusd should handle D-Bus connection errors gracefully rather than crashing the daemon process.
Contributor
|
To properly eliminate 2-second polling while maintaining total hardware stability, you can test out something like that: let manager3 = manager.clone();
tokio::spawn(async move {
// 1. Initial State Fetch & Apply at Daemon Startup (Fixes Boot Issue)
let mut last_power = match manager3.on_external_power().await {
Ok(power) => {
debug!("Initial AC power state on startup: {}", power);
on_external_power_change(power).await;
power
}
Err(e) => {
warn!("Failed to read initial AC power state from logind: {}", e);
false
}
};
// 2. Subscribe to D-Bus Property Change Stream
let mut stream = manager3.receive_on_external_power_changed().await;
// 3. Process Signals with Event Deduplication
while let Some(change) = stream.next().await {
if let Ok(external_power) = change.get().await {
if external_power != last_power {
last_power = external_power;
debug!("AC power state changed: {}", external_power);
on_external_power_change(external_power).await;
}
}
}
});
let manager4 = manager.clone();
tokio::spawn(async move {
// 1. Initial Lid State Fetch & Apply at Daemon Startup
let mut last_lid = match manager4.lid_closed().await {
Ok(closed) => {
debug!("Initial lid state on startup: {}", closed);
on_lid_change(closed).await;
closed
}
Err(e) => {
warn!("Failed to read initial lid state from logind: {}", e);
false
}
};
// 2. Subscribe to D-Bus Property Change Stream
let mut stream = manager4.receive_lid_closed_changed().await;
// 3. Process Signals with Event Deduplication
while let Some(change) = stream.next().await {
if let Ok(lid_closed) = change.get().await {
if lid_closed != last_lid {
last_lid = lid_closed;
debug!("Lid state changed: {}", lid_closed);
on_lid_change(lid_closed).await;
}
}
}
}); |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
asusd/src/lib.rs (1)
239-239: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid eager property-cache initialization during startup.
With zbus 5.18.0,
CacheProperties::Yeswaits for cache readiness duringbuild().awaitand propagates cache errors. The following.expect(...)then panics before the handled property reads. UseCacheProperties::Lazily, or handle proxy-build errors without panicking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@asusd/src/lib.rs` at line 239, Update the proxy builder around CacheProperties::Yes to use lazy property-cache initialization via CacheProperties::Lazily, preserving the existing handled property-read flow and avoiding startup panics from eager cache readiness errors.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@asusd/src/lib.rs`:
- Around line 279-282: Update the startup and subsequent property-read handling
for the cached AC-power and lid states in the relevant CtrlPlatform and
AniMeZbus callbacks to use Option<bool>, preserving None when reads fail. Log
every initial and later read error, reset the corresponding cache to None on
failure, and ensure the next valid signal is processed even when it is false
rather than suppressed as unchanged.
---
Outside diff comments:
In `@asusd/src/lib.rs`:
- Line 239: Update the proxy builder around CacheProperties::Yes to use lazy
property-cache initialization via CacheProperties::Lazily, preserving the
existing handled property-read flow and avoiding startup panics from eager cache
readiness errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 11914d2a-1003-4e2c-bb3f-c496ec40de86
📒 Files selected for processing (1)
asusd/src/lib.rs
📜 Review details
🔇 Additional comments (2)
asusd/src/lib.rs (2)
285-289: 🩺 Stability & AvailabilityVerify a fallback for systems without logind property signals.
After the startup read, both tasks wait only on
receive_*_changed(). If a supported logind/systemd combination does not emit these signals, later AC or lid transitions never reach the callbacks. Retain a low-rate fallback or a guaranteed event source, or narrow and test the supported systemd versions.This repeats the unresolved signal-availability concern from the previous review.
Also applies to: 315-319
21-25: LGTM!Also applies to: 272-278, 302-308
Comment on lines
+279
to
+282
| Err(e) => { | ||
| warn!("Failed to read initial AC power state from logind: {}", e); | ||
| false | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve an unknown state after read failures.
Lines 281 and 311 convert a failed startup read into false. If the first valid signal also reports false, Lines 291 and 321 suppress it as unchanged. The CtrlPlatform and AniMeZbus callbacks then do not apply the startup state.
The if let Ok(...) blocks also discard later property-read errors. Use Option<bool>, log each error, and set the cached state to None so the next valid value is processed.
Proposed state handling
- let mut last_power = match manager3.on_external_power().await {
+ let mut last_power = match manager3.on_external_power().await {
Ok(power) => {
debug!("Initial AC power state on startup: {}", power);
on_external_power_change(power).await;
- power
+ Some(power)
}
Err(e) => {
warn!("Failed to read initial AC power state from logind: {}", e);
- false
+ None
}
};
while let Some(change) = stream.next().await {
- if let Ok(external_power) = change.get().await {
- if external_power != last_power {
- last_power = external_power;
- debug!("AC power state changed: {}", external_power);
- on_external_power_change(external_power).await;
- }
+ match change.get().await {
+ Ok(external_power) if last_power != Some(external_power) => {
+ last_power = Some(external_power);
+ debug!("AC power state changed: {}", external_power);
+ on_external_power_change(external_power).await;
+ }
+ Ok(_) => {}
+ Err(e) => {
+ last_power = None;
+ warn!("Failed to read AC power state after logind change: {}", e);
+ }
}
}Apply the same pattern to last_lid.
Also applies to: 289-296, 309-312, 319-325
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@asusd/src/lib.rs` around lines 279 - 282, Update the startup and subsequent
property-read handling for the cached AC-power and lid states in the relevant
CtrlPlatform and AniMeZbus callbacks to use Option<bool>, preserving None when
reads fail. Log every initial and later read error, reset the corresponding
cache to None on failure, and ensure the next valid signal is processed even
when it is false rather than suppressed as unchanged.
Author
|
The suggested improvements seem good to me and they do solve the original problem I faced. I did discover that my laptop doesn't actually emit Monitor of `login1` during unplug and replug of ACMonitor of `UPower` during unplug and replug of AC: |
Ghoul4500
requested changes
Jul 31, 2026
Ghoul4500
left a comment
Ghoul4500
left a comment
Member
There was a problem hiding this comment.
As @scardracs mentioned:
OnExternalPower is populated via udev/sysfs power_supply rules, but logind's D-Bus interface vtable does not guarantee emits-change-signal="true" for all properties across all systemd versions.
This would indeed cause issues with Ac/battery transitions. Might be better to watch udev power_supply events instead.
Member
|
Also please keep commit history clean when you do bring these changes. Makes for easier review and allows me to rebase and merge more cleanly |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@asusd/src/lib.rs`:
- Around line 289-343: Extract the duplicated initial-read, subscription,
deduplication, and callback flow from the lid and external-power tasks into a
shared generic helper. Have the helper accept an async initial-read closure, the
property-change stream, and the callback, while preserving each source’s value
transformation and failure logging; use it from both tasks and retain the
required Option<bool> tracking behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c9d2dcb3-b456-4146-b50b-68a0dfc3e98d
📒 Files selected for processing (1)
asusd/src/lib.rs
📜 Review details
🔇 Additional comments (3)
asusd/src/lib.rs (3)
244-258: 🩺 Stability & Availability | ⚡ Quick win
.expect()still panics the daemon on D-Bus setup failure.
Connection::system(),ManagerProxy::builder()...build(), and the newUPowerProxy::builder()...build()all use.expect(). A D-Bus error during early boot (logind or UPower not yet available) crashes the daemon process instead of degrading gracefully. This is the same concern raised in a previous review round on this setup block, now also covering the newUPowerProxyline.
289-343: 🎯 Functional Correctness | ⚡ Quick winRead failures during the stream loop are still silently dropped, and initial failures still collapse to
false.Both tasks share the same gap flagged in a previous review round:
Errfromlogind_manager3.lid_closed()/upower_manager.on_battery()on Line 297 / Line 327 becomesfalsewith only awarn!at startup, and later in the loop,if let Ok(lid_closed) = change.get().await(Line 306) andif let Ok(on_battery) = change.get().await(Line 335) discardErrwith no log line at all. If the first valid signal after such a failure also carriesfalse, the dedup check on Line 307 / Line 337 suppresses it as unchanged, andon_lid_change/on_external_power_changenever receive the true state.Track the state as
Option<bool>, log everyErrfrom.get(), and reset the cache toNoneon failure so the next valid value is always applied.Proposed state handling (apply to both blocks)
- let mut last_lid = match logind_manager3.lid_closed().await { + let mut last_lid: Option<bool> = match logind_manager3.lid_closed().await { Ok(closed) => { debug!("Initial lid state on startup: {}", closed); on_lid_change(closed).await; - closed + Some(closed) } Err(e) => { warn!("Failed to read initial lid state from logind: {}", e); - false + None } }; let mut stream = logind_manager3.receive_lid_closed_changed().await; while let Some(change) = stream.next().await { - if let Ok(lid_closed) = change.get().await { - if lid_closed != last_lid { - last_lid = lid_closed; - debug!("Lid state changed: {}", lid_closed); - on_lid_change(lid_closed).await; - } + match change.get().await { + Ok(lid_closed) if last_lid != Some(lid_closed) => { + last_lid = Some(lid_closed); + debug!("Lid state changed: {}", lid_closed); + on_lid_change(lid_closed).await; + } + Ok(_) => {} + Err(e) => { + last_lid = None; + warn!("Failed to read lid state after logind change: {}", e); + } } }Apply the same pattern to
last_power/on_battery.
29-29: LGTM!Also applies to: 40-49, 260-271, 273-284
Comment on lines
289
to
343
| let mut last_lid = match logind_manager3.lid_closed().await { | ||
| Ok(closed) => { | ||
| debug!("Initial lid state on startup: {}", closed); | ||
| on_lid_change(closed).await; | ||
| closed | ||
| } | ||
| Err(e) => { | ||
| warn!("Failed to read initial lid state from logind: {}", e); | ||
| false | ||
| } | ||
| }; | ||
|
|
||
| // 2. Subscribe to D-Bus Property Change Stream | ||
| let mut stream = logind_manager3.receive_lid_closed_changed().await; | ||
|
|
||
| // 3. Process Signals with Event Deduplication | ||
| while let Some(change) = stream.next().await { | ||
| if let Ok(lid_closed) = change.get().await { | ||
| if lid_closed != last_lid { | ||
| last_lid = lid_closed; | ||
| debug!("Lid state changed: {}", lid_closed); | ||
| on_lid_change(lid_closed).await; | ||
| } | ||
| } | ||
| sleep(Duration::from_secs(2)).await; | ||
| } | ||
| }); | ||
|
|
||
| tokio::spawn(async move { | ||
| let mut last_lid = manager.lid_closed().await.unwrap_or_default(); | ||
| // need to loop on these as they don't emit signals | ||
| loop { | ||
| if let Ok(next) = manager.lid_closed().await { | ||
| if next != last_lid { | ||
| last_lid = next; | ||
| on_lid_change(next).await; | ||
| // 1. Initial State Fetch & Apply at Daemon Startup | ||
| let mut last_power = match upower_manager.on_battery().await { | ||
| Ok(on_battery) => { | ||
| let on_external_power = !on_battery; | ||
| debug!("Initial AC power state on startup: {}", on_external_power); | ||
| on_external_power_change(on_external_power).await; | ||
| on_external_power | ||
| } | ||
| Err(e) => { | ||
| warn!("Failed to read initial AC power state from upower: {}", e); | ||
| false | ||
| } | ||
| }; | ||
| // 2. Subscribe to D-Bus Property Change Stream | ||
| let mut stream = upower_manager.receive_on_battery_changed().await; | ||
|
|
||
| // 3. Process Signals with Event Deduplication | ||
| while let Some(change) = stream.next().await { | ||
| if let Ok(on_battery) = change.get().await { | ||
| let on_external_power = !on_battery; | ||
| if on_external_power != last_power { | ||
| last_power = on_external_power; | ||
| debug!("AC power state changed: {}", on_external_power); | ||
| on_external_power_change(on_external_power).await; | ||
| } | ||
| } | ||
| sleep(Duration::from_secs(2)).await; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract a shared helper for the repeated fetch-then-stream pattern.
The lid-change task (Lines 288-313) and the external-power task (Lines 317-343) implement the same skeleton: read the initial value, log on failure, subscribe to the change stream, dedupe against the last value, and invoke a callback. Only the property source (logind_manager3.lid_closed() vs !upower_manager.on_battery()) and the callback differ. Extract a small generic helper that takes an async initial-read closure, a stream, and a callback, to remove the duplication and keep the fix in one place once the Option<bool> tracking above is applied.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@asusd/src/lib.rs` around lines 289 - 343, Extract the duplicated
initial-read, subscription, deduplication, and callback flow from the lid and
external-power tasks into a shared generic helper. Have the helper accept an
async initial-read closure, the property-change stream, and the callback, while
preserving each source’s value transformation and failure logging; use it from
both tasks and retain the required Option<bool> tracking behavior.
Author
Was there something with the current commit history you wanted addressed or just a precautionary comment?
I commited a first take at this, this approach works for me in detecting AC state changes. asusctl logsI dont know if this warning is related to the delay? I initially looked at using upower_dbus but it is using an old version of zbus which depends on an old version of rustix that doesn't compile anymore. #[proxy(
interface = "org.freedesktop.UPower",
default_service = "org.freedesktop.UPower",
default_path = "/org/freedesktop/UPower"
)]
trait UPower {
#[zbus(property)]
fn on_battery(&self) -> zbus::Result<bool>;
}which I clumsily dropped near the top of UPower also provides lid state changes via I also don't know that this is even a portable solution, or if some combination of UPower and logind is the solution. |
Member
I was pointing out that you were making a new commit for addressing review comments instead of doing fixup onto the original commit and force pushing. You should amend the original commit as force push in this case Also as per our commit convention you can write the message as "fix: [rest of message]" or similar I will review this again shortly. Thank you for your work on this |
Ghoul4500
requested changes
Jul 31, 2026
Ghoul4500
left a comment
Ghoul4500
left a comment
Member
There was a problem hiding this comment.
I still don't recommend this approach. Not all devices may have UPower. A udev monitor is probably the best here still. If you are unfamiliar with it I could bring the changes to this PR myself and squash merge
omuhr
force-pushed
the
reduce-logind-spam
branch
2 times, most recently
from
July 31, 2026 23:27
56f7b05 to
591a074
Compare
Instead of polling OnExternalPower and LidClosed every 2 seconds via D-Bus property calls, subscribe to PropertiesChanged signals from logind. This eliminates constant D-Bus traffic to logind which was causing slowdowns for other logind callers (e.g. podman, busctl GetUser). Fixes OpenGamingCollective#235 update with suggested improvements move to upower for getting external power state (first draft) move to udev monitoring debloat forgot
omuhr
force-pushed
the
reduce-logind-spam
branch
from
July 31, 2026 23:31
591a074 to
e8587c9
Compare
Author
I had missed your recommendation of udev :) I took a stab at it, I wasn't quite sure how to handle the multi-threading of udev::MonitorSocket and error management in the thread in general. Feedback appreciated! |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@asusd/src/lib.rs`:
- Around line 428-430: Update the external power supply monitoring fallback in
the surrounding initialization flow to use error-level logging instead of
warn-level logging, and include the specific failure cause: either
/sys/class/power_supply was unreadable or no power-supply entry had type
"Mains".
- Around line 388-421: Update the change-event handling around
power_supply_monitor.iter() to fall back to reading the external power supply’s
sysfs online value when property_value("POWER_SUPPLY_ONLINE") is absent. Use the
matched device sysname to read /sys/class/power_supply/<sysname>/online, parse
its value into the same boolean state, and only warn and skip when both sources
are unavailable or invalid; preserve the existing state-change comparison and
on_external_power_change call.
- Around line 322-337: Update the external power-supply initialization around
the `and_then` chain to keep a discovered mains supply even when reading its
initial `online` file fails; log the read error and represent the initial state
as unknown rather than returning `None`. Start the udev monitor whenever supply
discovery succeeds, and change `last_power_supply_state` in the monitor thread
to `Option<bool>` so unknown initial state is handled without suppressing future
change events.
- Around line 344-345: Update the external-power handling around
tokio::runtime::Handle::current and the spawned udev monitor thread so it sends
power-state changes through a channel instead of synchronously waiting for the
callback. Add a Tokio task that receives channel messages and performs the
potentially slow AniMe USB callback, keeping the udev polling loop non-blocking
and preserving each power-state change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d30161fe-61cb-47d7-b787-34fe112d71a9
📒 Files selected for processing (1)
asusd/src/lib.rs
📜 Review details
🔇 Additional comments (6)
asusd/src/lib.rs (6)
234-242:Connection::system()andManagerProxy::builder(...).build()still panic the daemon on failure. This was raised in a previous review on this same range.
278-301: The failure path still storesfalseas a real state, so a firstfalsesignal is suppressed andon_lid_changenever runs. Errors fromchange.get()are also discarded. UseOption<bool>and log the error. This was raised in a previous review.
24-24: LGTM!
244-272: LGTM!
346-386: LGTM!
310-321: 🎯 Functional CorrectnessConfirm whether supported ASUS hardware can expose multiple
Mainssupplies.Linux permits multiple power-supply devices, and drivers can classify USB-PD ports as
Mains. This code selects one sysname and drops events from the others. If any supported model exposes multipleMainssupplies, collect all matching sysnames and compute the logical OR of theironlinevalues.
Comment on lines
+322
to
+337
| .and_then(|sysname| { | ||
| // Look up the initial state of the external power supply | ||
| let path = std::path::PathBuf::from("/sys/class/power_supply") | ||
| .join(&sysname) | ||
| .join("online"); | ||
| let state = std::fs::read_to_string(&path) | ||
| .map_err(|e| { | ||
| error!( | ||
| "Could not read external power supply state from {path:?}: {e}" | ||
| ) | ||
| }) | ||
| .ok() | ||
| .map(|s| s.trim() != "0")?; | ||
| Some((sysname, state)) | ||
| }) | ||
| { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not disable monitoring when only the initial online read fails.
The second and_then returns None if reading online fails. The whole if let Some(...) block is then skipped, so the udev monitor never starts and no external-power change is ever reported for the rest of the daemon lifetime. A transient read error should not disable event monitoring permanently.
Separate supply discovery from the initial state read. Start the monitor whenever a mains supply is found, and treat an unreadable initial state as unknown.
🛠️ Proposed restructure
- if let Some((external_power_supply_sysname, initial_power_supply_state)) =
+ if let Some(external_power_supply_sysname) =
std::fs::read_dir("/sys/class/power_supply")
.ok()
.and_then(|dir| {
// Look up the external power supply sysname (e.g. "ACAD") by finding one
// with the type "mains"
dir.flatten().find_map(|entry| {
let type_path = entry.path().canonicalize().ok()?.join("type");
let supply_type = std::fs::read_to_string(type_path).ok()?;
supply_type
.trim()
.eq_ignore_ascii_case("mains")
.then(|| entry.file_name().to_string_lossy().to_string())
})
})
- .and_then(|sysname| {
- // Look up the initial state of the external power supply
- let path = std::path::PathBuf::from("/sys/class/power_supply")
- .join(&sysname)
- .join("online");
- let state = std::fs::read_to_string(&path)
- .map_err(|e| {
- error!(
- "Could not read external power supply state from {path:?}: {e}"
- )
- })
- .ok()
- .map(|s| s.trim() != "0")?;
- Some((sysname, state))
- })
{
- debug!(
- "External power supply plugged in on startup: {}",
- initial_power_supply_state
- );
- on_external_power_change(initial_power_supply_state).await;
+ let online_path = std::path::PathBuf::from("/sys/class/power_supply")
+ .join(&external_power_supply_sysname)
+ .join("online");
+ let mut initial_power_supply_state = match std::fs::read_to_string(&online_path) {
+ Ok(s) => Some(s.trim() != "0"),
+ Err(e) => {
+ error!("Could not read external power supply state from {online_path:?}: {e}");
+ None
+ }
+ };
+ if let Some(state) = initial_power_supply_state {
+ debug!("External power supply plugged in on startup: {}", state);
+ on_external_power_change(state).await;
+ }Then track last_power_supply_state as Option<bool> in the monitor thread.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@asusd/src/lib.rs` around lines 322 - 337, Update the external power-supply
initialization around the `and_then` chain to keep a discovered mains supply
even when reading its initial `online` file fails; log the read error and
represent the initial state as unknown rather than returning `None`. Start the
udev monitor whenever supply discovery succeeds, and change
`last_power_supply_state` in the monitor thread to `Option<bool>` so unknown
initial state is handled without suppressing future change events.
Comment on lines
+344
to
+345
| let handle = tokio::runtime::Handle::current(); | ||
| std::thread::spawn(move || { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the tokio runtime construction and the tokio feature set for asusd.
fd -t f 'Cargo.toml' | xargs rg -n -C3 'tokio'
# Find the runtime entry point / macro attributes.
rg -nP -C5 '#\[tokio::main[^\]]*\]|Builder::new_(multi_thread|current_thread)|Runtime::new\(\)' --type=rustRepository: OpenGamingCollective/asusctl
Length of output: 2306
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workspace Tokio features ---'
sed -n '58,78p' Cargo.toml
printf '%s\n' '--- asusd entry points and runtime setup ---'
rg -nP -C8 '#\[tokio::main[^\]]*\]|Builder::new_(multi_thread|current_thread)|Runtime::new\(\)|fn main' asusd --glob '*.rs'
printf '%s\n' '--- target call sites ---'
sed -n '320,435p' asusd/src/lib.rs
printf '%s\n' '--- monitor and callback implementations ---'
rg -n -C8 'poll\.poll|on_external_power_change|external_power|AniMe|hid|usb' asusd/src --glob '*.rs'Repository: OpenGamingCollective/asusctl
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- create_sys_event_tasks and caller ---'
sed -n '190,345p' asusd/src/lib.rs
printf '%s\n' '--- external-power callback wiring ---'
rg -n -C10 'create_sys_event_tasks|set_power_states|set_power|external_power' asusd/src --glob '*.rs'
printf '%s\n' '--- AniMe power callback methods ---'
rg -n -C12 'set_power_states|power_state|external_power|on_external' asusd/src/aura_anime --glob '*.rs'
printf '%s\n' '--- AniMe implementation outline and relevant body ---'
ast-grep outline asusd/src/aura_anime/trait_impls.rsRepository: OpenGamingCollective/asusctl
Length of output: 44013
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
workspace = Path("Cargo.toml").read_text()
daemon = Path("asusd/src/daemon.rs").read_text()
lib = Path("asusd/src/lib.rs").read_text()
anime = Path("asusd/src/aura_anime/trait_impls.rs").read_text()
features = re.search(r'tokio\s*=\s*\{.*?features\s*=\s*\[(.*?)\]', workspace, re.S)
assert features and '"rt-multi-thread"' in features.group(1), "workspace lacks rt-multi-thread"
assert re.search(r'#\[tokio::main\]\s*async\s+fn\s+main', daemon), "asusd does not use the default Tokio main macro"
assert "handle.block_on(on_external_power_change" in lib, "target block_on call not found"
callback = re.search(
r'move \|power_plugged\|.*?async move \{(.*?)\n\s*\},',
anime,
re.S,
)
assert callback, "AniMe external-power callback not found"
assert ".write_bytes(" in callback.group(1), "callback has no device write"
assert ".await" in callback.group(1), "callback has no awaited operation"
print("runtime_flavor=multi_thread (workspace feature + default #[tokio::main])")
print("foreign_thread_block_on=present")
print("anime_external_power_callback=async device writes present")
PYRepository: OpenGamingCollective/asusctl
Length of output: 329
Move external-power handling off the udev monitor thread.
asusd uses Tokio's multi-thread runtime, so Handle::block_on from this thread is valid. However, the call blocks until the callback completes. Slow AniMe USB writes can delay poll.poll, allowing the udev socket queue to overflow and lose power-state changes. Send state changes through a channel to a Tokio task instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@asusd/src/lib.rs` around lines 344 - 345, Update the external-power handling
around tokio::runtime::Handle::current and the spawned udev monitor thread so it
sends power-state changes through a channel instead of synchronously waiting for
the callback. Add a Tokio task that receives channel messages and performs the
potentially slow AniMe USB callback, keeping the udev polling loop non-blocking
and preserving each power-state change.
Comment on lines
+388
to
421
| for event in power_supply_monitor.iter() { | ||
| if event.event_type() != udev::EventType::Change { | ||
| continue; | ||
| } | ||
| if event.device().sysname().to_string_lossy() | ||
| != external_power_supply_sysname | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| let Some(current_power_supply_state) = event | ||
| .device() | ||
| .property_value("POWER_SUPPLY_ONLINE") | ||
| .map(|v| v != "0") | ||
| else { | ||
| warn!( | ||
| "Power supply change event for external power supply \ | ||
| missing POWER_SUPPLY_ONLINE property, skipping..." | ||
| ); | ||
| continue; | ||
| }; | ||
|
|
||
| if current_power_supply_state != last_power_supply_state { | ||
| last_power_supply_state = current_power_supply_state; | ||
| debug!( | ||
| "External power supply state changed: {}", | ||
| current_power_supply_state | ||
| ); | ||
| handle.block_on(on_external_power_change( | ||
| current_power_supply_state, | ||
| )); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Re-read online from sysfs instead of relying only on the uevent property.
The handler skips any change event that does not carry POWER_SUPPLY_ONLINE. The kernel does not guarantee that every change uevent for a power supply includes the full property set. When the property is missing, the daemon logs a warning and keeps the previous state, so a real transition can be lost permanently.
Fall back to reading /sys/class/power_supply/<sysname>/online when the property is absent.
🛠️ Proposed fallback
- let Some(current_power_supply_state) = event
- .device()
- .property_value("POWER_SUPPLY_ONLINE")
- .map(|v| v != "0")
- else {
- warn!(
- "Power supply change event for external power supply \
- missing POWER_SUPPLY_ONLINE property, skipping..."
- );
- continue;
- };
+ let device = event.device();
+ let current_power_supply_state = match device
+ .property_value("POWER_SUPPLY_ONLINE")
+ .map(|v| v != "0")
+ {
+ Some(state) => state,
+ None => {
+ let path = std::path::PathBuf::from(
+ "/sys/class/power_supply",
+ )
+ .join(&external_power_supply_sysname)
+ .join("online");
+ match std::fs::read_to_string(&path) {
+ Ok(s) => s.trim() != "0",
+ Err(e) => {
+ warn!(
+ "Power supply change event missing \
+ POWER_SUPPLY_ONLINE and {path:?} is \
+ unreadable: {e}"
+ );
+ continue;
+ }
+ }
+ }
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for event in power_supply_monitor.iter() { | |
| if event.event_type() != udev::EventType::Change { | |
| continue; | |
| } | |
| if event.device().sysname().to_string_lossy() | |
| != external_power_supply_sysname | |
| { | |
| continue; | |
| } | |
| let Some(current_power_supply_state) = event | |
| .device() | |
| .property_value("POWER_SUPPLY_ONLINE") | |
| .map(|v| v != "0") | |
| else { | |
| warn!( | |
| "Power supply change event for external power supply \ | |
| missing POWER_SUPPLY_ONLINE property, skipping..." | |
| ); | |
| continue; | |
| }; | |
| if current_power_supply_state != last_power_supply_state { | |
| last_power_supply_state = current_power_supply_state; | |
| debug!( | |
| "External power supply state changed: {}", | |
| current_power_supply_state | |
| ); | |
| handle.block_on(on_external_power_change( | |
| current_power_supply_state, | |
| )); | |
| } | |
| } | |
| } | |
| for event in power_supply_monitor.iter() { | |
| if event.event_type() != udev::EventType::Change { | |
| continue; | |
| } | |
| if event.device().sysname().to_string_lossy() | |
| != external_power_supply_sysname | |
| { | |
| continue; | |
| } | |
| let device = event.device(); | |
| let current_power_supply_state = match device | |
| .property_value("POWER_SUPPLY_ONLINE") | |
| .map(|v| v != "0") | |
| { | |
| Some(state) => state, | |
| None => { | |
| let path = std::path::PathBuf::from( | |
| "/sys/class/power_supply", | |
| ) | |
| .join(&external_power_supply_sysname) | |
| .join("online"); | |
| match std::fs::read_to_string(&path) { | |
| Ok(s) => s.trim() != "0", | |
| Err(e) => { | |
| warn!( | |
| "Power supply change event missing \ | |
| POWER_SUPPLY_ONLINE and {path:?} is \ | |
| unreadable: {e}" | |
| ); | |
| continue; | |
| } | |
| } | |
| } | |
| }; | |
| if current_power_supply_state != last_power_supply_state { | |
| last_power_supply_state = current_power_supply_state; | |
| debug!( | |
| "External power supply state changed: {}", | |
| current_power_supply_state | |
| ); | |
| handle.block_on(on_external_power_change( | |
| current_power_supply_state, | |
| )); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@asusd/src/lib.rs` around lines 388 - 421, Update the change-event handling
around power_supply_monitor.iter() to fall back to reading the external power
supply’s sysfs online value when property_value("POWER_SUPPLY_ONLINE") is
absent. Use the matched device sysname to read
/sys/class/power_supply/<sysname>/online, parse its value into the same boolean
state, and only warn and skip when both sources are unavailable or invalid;
preserve the existing state-change comparison and on_external_power_change call.
Comment on lines
+428
to
+430
| } else { | ||
| warn!("External power supply monitoring unavailable"); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Raise the log level and name the failure cause.
If no mains supply is found, external-power tracking is silently disabled for the whole daemon lifetime. Power profiles, fan curves, and dGPU power states then never react to AC changes. This is a functional loss, not a routine condition.
Use error! and include the reason, for example that /sys/class/power_supply was unreadable or that no entry has type == "Mains".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@asusd/src/lib.rs` around lines 428 - 430, Update the external power supply
monitoring fallback in the surrounding initialization flow to use error-level
logging instead of warn-level logging, and include the specific failure cause:
either /sys/class/power_supply was unreadable or no power-supply entry had type
"Mains".
Member
|
@omuhr This looks much better now. Coderabbit has flagged everything to take care of so I will wait for you to fix those |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Instead of polling OnExternalPower and LidClosed every 2 seconds via D-Bus property calls, subscribe to PropertiesChanged signals from logind.
This eliminates constant D-Bus traffic to logind which was causing slowdowns for other logind callers (e.g. podman, busctl GetUser).
Fixes #235
Monitor of the dbus with the old binary
Logs of the old binary
Monitor of the dbus with the new binary
Logs of the new binary
Note the time separation of the events in the monitor for the new binary, the timestamps correspond to when I closed and opened the lid.
Tested Hardware & Environment
Verification and testing:
cargo fmt --all -- --check)cargo clippy --all -- -D warnings/cargo check --all-targets)cargo test --all)cargo cranky)